Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add websocket entrypoint for app #26

Merged
merged 32 commits into from
May 28, 2024
Merged

Conversation

Sotatek-JohnnyNguyen
Copy link
Contributor

No description provided.

src/events.module.ts Outdated Show resolved Hide resolved
@@ -23,4 +23,6 @@ export default () => ({
/^eth_.*Filter$/,
],
inheritHostHeader: true,
wsPort: process.env.WS_PORT,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sotatek-JohnnyNguyen
Is "wsPort" being used anywhere? If it is not being used, please remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i see

return false;
}
return true;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sotatek-JohnnyNguyen
If it were me, I would write the code as shown below.
Is there any reason you have written it this way?

  isConnected() {
    return (this.socket.readyState == this.socket.OPEN);
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry! you're right, let me change this

.env.example Outdated
VERSE_READ_NODE_URL=http://localhost:8545
BLOCK_NUMBER_CACHE_EXPIRE_SEC=
DATASTORE=
NODE_SOCKET=ws://[::]:8546

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sotatek-JohnnyNguyen
.env.example file has been added, but how is it intended to be used?
It appears that dotenv is not included in the package.json.
Will you use the source .env command?

And it would be helpful if you could also update the README.md file to reflect the current situation.

Although it is unrelated to this change, I believe that $ npm build in the README.md file should be corrected to $ npm run build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First: Nestjs provide configModule that exposes a configService which loads the appropriate .env file, so we dont need dotenv for now
Second: the .env.example is like an example to help other devs know which configuration variables they have to add to .env file in order to run the service

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update README.me file now. Also yes both commands $ npm build and $ npm run build are doing the same action

Copy link

@georgefuru georgefuru May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sotatek-JohnnyNguyen
Okay. Thanks for replying.

First: Nestjs provide configModule that exposes a configService which loads the appropriate .env file, so we dont need dotenv for now

Good! Thanks.

Second: the .env.example is like an example to help other devs know which configuration variables they have to add to .env file in order to run the service

Of course, I know that. My way of asking the question might not have been good.

By the way, what do you think about including the environment variable PORT=, which is currently written to be exported in the README.md, in the .env.example file?
If there are no issues, I feel it would be good to add it.

Copy link
Contributor Author

@Sotatek-JohnnyNguyen Sotatek-JohnnyNguyen May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

By the way, what do you think about including the environment variable PORT=, which is currently written to be exported in the README.md, in the .env.example file?
If there are no issues, I feel it would be good to add it.

README.md Outdated
MAXRECONNECTATTEMPTS: 10
```

### 5. Set up npm

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sotatek-JohnnyNguyen
There are two '5.'s.

5. Handle disconnect from node's websocket

5. Set up npm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, my bad. let me fix this

.env.example Outdated
DATASTORE=redis
NODE_SOCKET=ws://[::]:8546
PORT=3000
REDIS_URI=redis://localhost:6373

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sotatek-JohnnyNguyen
I believe the default port for Redis is 6379. Is there a specific reason it was changed to 6373?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used 6373 for my redis

Copy link

@georgefuru georgefuru May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sotatek-JohnnyNguyen
https://github.com/oasysgames/verse-proxy/blob/fc1b97ce63bcb370bffbda97419523cdb95c89c8/docs/RateLimit.md?plain=1

Since the above mentions REDIS, please adjust the README as needed.

https://github.com/oasysgames/verse-proxy/blob/master/docs/RateLimit.md

I think it would be preferable to list DATASTORE and REDIS_URI side by side.
And Redis' default port is 6379, so I think
REDIS_URI=redis://localhost:6379
is better.

Copy link

@georgefuru georgefuru May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sotatek-JohnnyNguyen
Have you checked the above comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have, but from my perspective, I don't think we need to change anything here since the README tells what it needs to tell about the datastore config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review my commit

Copy link
Collaborator

@ironbeer ironbeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a request routing features to master node and reader node (replica node) similar to an HTTP proxy. Do not need to consider session affinity for WebSocket sessions.(Do not need to consider session affinity for WS sessions.)

When only VERSE_URL (also known as VERSE_MASTER_NODE_URL) is set
Proxy all requests to the master node.

When VERSE_READ_NODE_URL is also set
Proxy eth_sendRawTransaction and eth_estimateGas requests to the master node.
Proxy all other requests to the reader node.

Code: https://github.com/oasysgames/verse-proxy/blob/features/add-support-web-socket/src/services/proxy.service.ts#L84-L104

Copy link
Collaborator

@ironbeer ironbeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the environment variable for the WebSocket origin address does not configured, the process will crash. Since the WebSocket proxy is an optional feature, please ensure it is only enabled if the environment variable is present.

@Sotatek-JohnnyNguyen
Copy link
Contributor Author

Sotatek-JohnnyNguyen commented May 17, 2024

Need a request routing features to master node and reader node (replica node) similar to an HTTP proxy. Do not need to consider session affinity for WebSocket sessions.(Do not need to consider session affinity for WS sessions.)

When only VERSE_URL (also known as VERSE_MASTER_NODE_URL) is set Proxy all requests to the master node.

When VERSE_READ_NODE_URL is also set Proxy eth_sendRawTransaction and eth_estimateGas requests to the master node. Proxy all other requests to the reader node.

Code: https://github.com/oasysgames/verse-proxy/blob/features/add-support-web-socket/src/services/proxy.service.ts#L84-L104

That means the verse-proxy may have to keep 2 web-sockets connection to master node and reader node right?

@ironbeer
Copy link
Collaborator

Need a request routing features to master node and reader node (replica node) similar to an HTTP proxy. Do not need to consider session affinity for WebSocket sessions.(Do not need to consider session affinity for WS sessions.)
When only VERSE_URL (also known as VERSE_MASTER_NODE_URL) is set Proxy all requests to the master node.
When VERSE_READ_NODE_URL is also set Proxy eth_sendRawTransaction and eth_estimateGas requests to the master node. Proxy all other requests to the reader node.
Code: https://github.com/oasysgames/verse-proxy/blob/features/add-support-web-socket/src/services/proxy.service.ts#L84-L104

That means the verse-proxy may have to keep 2 web-sockets connection to master node and reader node right?

yes

Copy link
Collaborator

@ironbeer ironbeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sotatek-JohnnyNguyen I found a bug that causes mixed responses when multiple clients are connected.
If you build and run the test command in this repository(or using ethers.js, etc.), it will subscribe to newHeads, so if you create another connection with wscat, etc. in that state, the response should show up.

Additionally, I found an issue where subscriptions from disconnected clients remain and responses are being sent to other clients. This phenomenon can also be reproduced with the previously mentioned command.

@ironbeer ironbeer merged commit bf0e2f4 into master May 28, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants